feat: multichain support #38
Conversation
| for (const orchestrator of this.orchestrators.values()) { | ||
| this.logger.info(`Starting orchestrator for chain ${orchestrator.chainId}...`); | ||
| orchestratorProcesses.push(orchestrator.run(abortController.signal)); | ||
| } |
There was a problem hiding this comment.
| for (const orchestrator of this.orchestrators.values()) { | |
| this.logger.info(`Starting orchestrator for chain ${orchestrator.chainId}...`); | |
| orchestratorProcesses.push(orchestrator.run(abortController.signal)); | |
| } | |
| const orchestratorProcesses = this.orchestrators.values().map(orchestrator=>orchestrator.run); |
There was a problem hiding this comment.
but we're not passing the AbortController here, the AbortController is so we can gracefully exit the app so it finishes the current event being processed and then exits the app
There was a problem hiding this comment.
it's missing in my suggestion but you can xd
| orchestratorProcesses.push(orchestrator.run(abortController.signal)); | ||
| } | ||
|
|
||
| await Promise.allSettled(orchestratorProcesses); |
There was a problem hiding this comment.
Lets keep in mind that we should make all the orchestrators to be infinite running processes, never throw errors, and have automatic recovery. This is to avoid affecting the others. In case we have inifinite running processes and never throw errors, then Promise.allSettled or Promise.all both should be the same since none of the processes resolve. Apart from automatic recovery we should notify once there is a critical error that kills the specific orchestrator.
There was a problem hiding this comment.
agree that all and allSettled are the same here. You're suggesting to use all so we can catch early a possible Fatal error that killed the Orchestrator?
There was a problem hiding this comment.
Should be the same using allSettled or all because all are infinite processes and each of them should avoid bubbling up errors
| // Initialize EVM provider | ||
| const evmProvider = new EvmProvider(env.RPC_URLS, optimism, this.logger); | ||
| for (const chain of chains) { | ||
| const chainLogger = new Logger({ chainId: chain.id as ChainId }); |
There was a problem hiding this comment.
We need to initialize a logger for each chain?
There was a problem hiding this comment.
we come up to this change because we want to distinguish logs for each chain (adding some identifier string as part of general formatting)
| @@ -55,13 +55,22 @@ export class SharedDependenciesService { | |||
| kyselyDatabase, | |||
| env.DATABASE_SCHEMA, | |||
| ); | |||
| const pricingProvider = PricingProviderFactory.create(env, { logger }); | |||
| const pricingProvider = PricingProviderFactory.create(env, { | |||
| logger: new Logger({ className: "PricingProvider" }), | |||
There was a problem hiding this comment.
Similar to my last comment, do you think it's better not to use DI for the logger?
There was a problem hiding this comment.
mmm could be 🤔 but at the same time i like DI... wdyt @0xkenj1
| import { Orchestrator } from "@grants-stack-indexer/data-flow"; | ||
|
|
||
| import type { Environment } from "../../src/config/env.js"; | ||
| import { ProcessingService } from "../../src/services/processing.service.js"; |
There was a problem hiding this comment.
yeah, however i don't think is much of a problem in test files
🤖 Linear
Closes GIT-177 GIT-178 GIT-179
Description
Checklist before requesting a review